Fix RemoteExecutor for VS testhost #7426
Conversation
| Path = typeof(RemoteExecutor).Assembly.Location; | ||
|
|
||
| if (IsNetCore()) | ||
| { | ||
| HostRunner = processFileName; | ||
|
|
||
| // Running with an apphost, eg. Visual Studio testhost. We should attempt to find and use dotnet.exe. | ||
| // This is a Windows-only workaround. |
There was a problem hiding this comment.
Is the only Windows-specific part of the workaround the .exe suffix? Can we look for dotnet.exe on Windows and dotnet everywhere else?
There was a problem hiding this comment.
I have no time / opportunity to validate the change with Unix test hosts, therefore I'm keeping this fix scoped to Windows + VS testhost intentionally. I don't want to add general code I cannot test.
There was a problem hiding this comment.
If you keep this Windows only, do you also need to re-enable the tests on Windows only and keep them disabled on non-Windows? I do not think it is worth the troubles.
The Windows specific logic is very minimal. I would be ok with adding it without you explicitly validating it. Add string hostName = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? "dotnet.exe" : dotnet"; and use it throughout the change instead of hardcoding dotnet.exe.
| @@ -10,6 +10,7 @@ | |||
| using System.Runtime.InteropServices; | |||
| using System.Threading.Tasks; | |||
| using Xunit; | |||
| using IOPath = System.IO.Path; | |||
There was a problem hiding this comment.
Why not just use using System.IO; and Path?
There was a problem hiding this comment.
RemoteExecutor has a field named Path.
Tbh I don't think that validation process makes much sense. What would be very useful to test is the integration in dotnet/runtime. Can you please push the locally built RemoteExecutor package to any feed so that we can then submit a PR in dotnet/runtime to consume and test it? (by adding the feed into the NuGet.config file and updating the version in Versions.props) If you don't have a feed handy let me know and I can push it to mine (myget which is free). I doubt that this would break dotnet/runtime's usage of RemoteExecutor but better check before merging... |
|
I did the validation as requested. All tests passed in dotnet/runtime#53125, hopefully not as false positives 😄 |
Co-authored-by: Igor Velikorossov <RussKie@users.noreply.github.com>
|
Thanks a lot for the valuable contribution @antonfirsov 👍 |
Description
Address #6371 for cases when VS testhost is the runner.
The fix is Windows-specific./cc @vitek-karas @ViktorHofer @JimBobSquarePants @RussKie
To double check:
The validation process above looks very complicated, I have no opportunity to go through it. Note that the change enables disabled tests. I did a local run with both Visual Studio and
dotnet test.